Можете оценить код?

Question

[Игорь Миронов]

Это программа для бэкапа папок/файлов

import shutil
import time
from datetime import datetime

i = 0
t = 3 #Сбда вписать раз в сколько СЕКУНД делать бэкап

while 1 == 1:

	dt = datetime.today().strftime("%Y-%m-%d-%H.%M.%S")
	name = "backup_"

	new_name = name + str(i)

	directory_copy = 'C:\\Users\\MinuteX\\Downloads\\This is a server of MC\\fantasy_city' #Вписать директория откуда копировать папку ||| ВАЖНО!!! С ДВОЙНЫМ СЛЕШОМ
	directory = 'C:\\Users\\MinuteX\\Downloads\\Safe' #Впмсать директорию куда будут копироватся файлы, ||| ВАЖНО!!! ВСЕ СЛЕШЫ ДОЛЖНЫ БЫТЬ ДВОЙНЫМИ

	v_DIR = directory + '\\' + new_name + "_" + dt
	i += 1


	shutil.make_archive(v_DIR,
	                    'zip',
	                     directory_copy)

	print("Generate new backup: " + v_DIR)
	time.sleep(t)

Comments

[o5a]

константы лучше вынести из цикла наружу

Answer 1

[Владимир Куц]

Начните с приведения кода в соотвествии со стандартами pep8


Беглым взглядом:
while 1 == 1:
лучше заменить на
while True:
зачем вызывать операцию сравнения, если результат и так известен...

directory_copy = 'C:\\Users\\MinuteX\\Downloads\\This is a server of MC\\fantasy_city' #Вписать директория откуда копировать папку ||| ВАЖНО!!! С ДВОЙНЫМ СЛЕШОМ
directory = 'C:\\Users\\MinuteX\\Downloads\\Safe' #Впмсать директорию куда будут копироватся файлы, ||| ВАЖНО!!! ВСЕ СЛЕШЫ ДОЛЖНЫ БЫТЬ ДВОЙНЫМИ

почитайте за os.path.join

v_DIR = directory + '\\' + new_name + "_" + dt
Почитайте за os.path.join и f-строки

Всю вашу конструкцию в 5 строк можно привести к такому более читабельному виду к примеру:

destination_file = os.path.join(
    DESTINATION_DIR,
    f'backup_{i}_{datetime.today().strftime("%Y-%m-%d-%H.%M.%S")}',
)

directory_copy = ...
directory = ...

вынести за пределы цикла - в цикле вы присваиваете им циклически одно и то же значение, и дать более осмысленные наименования

В логику - не вникал...

Answer 2

[Сергей П]

К тому, что перечислил @fox_1 добавлю:

1. используйте pathlib;
   
2. выносите магические константы ("%Y-%m-%d-%H.%M.%S") в начало модуля, именуйте их внятно;
   
3. используйте argparse;
   
4. принимайте пути ('C:\\Users\\MinuteX\\Downloads\\This is a server of MC\\fantasy_city') в параметрах, можно использовать безопасные умолчания и относительные пути;
   
5. не конкатенируйте строки (а особенно пути) плюсиком, используйте f-строки или .format(...);
   
6. соберите код в функции, сделайте точку входа, анализируйте аргументы командной строки - даже в простых скриптах эта привычка даст вам много очков к опыту и привьёт правильный аккуратный подход к разработке инструментов;
   
7. в простых скриптах print - это нормально, но лучше использовать logging, чтобы ваш код можно было использовать как модуль и переопределить логированаие;
   
8. делайте выхлоп ошибок в stderr, а всякий полезный выхлоп в stdout, это позволит в командной строке на уровне пайпов направить логи работы вышего скрипта в разные файлы или потоки;
   
9. не рассчитывайте, что ваш код будет работать вечно и стабильно. Сейчас у вас период бэкапа нестабильный, зависит от продолжительности самого бэкапа - это, как минимум, странно и неожиданно. Я бы такую штуку сделал простой и одношговой, то есть при запуске утилита делает один единственный бэкап и завершается с ошибкой, если что-то пошло не так. Это позволит занести её в crontab или в планировщик задач, а результаты работы и успешность запуска оценивать внешними механизмами;
   
10. старайтесь всегда делать кросс-платформенный код, даже если он вряд ли будет запускаться на других ОС, даже если у вас нет времени протестировать его на разных ОС -- это сформирует у вас хорошую привычку и расширит кругозор, а также позволит не делать лишних специфических костылей.
    

Comments

[Andy_U]

не конкатенируйте строки (а особенно пути) плюсиком, используйте f-строки или .format(...);

Это почему?

[Сергей П]

Andy_U, ну строки в питоне иммутабельный тип (неизменяемый). Это значит, что нельзя, например, взять и заменить некоторые символы внутри строки. В памяти до сбора мусора останутся и обе части соединённых конкатенацией строк и новая соединённая копия. Представьте, что вы решили накопить конкатенацией длинную строку по одному символу. Например строку в сотню килобайт длиной (какой-нибудь длинный XML).
В памяти это будет итеративно занимать 1+2+3+4+5+6+..+N = N*(N-1)/2 байт. Это уже квадратичная сложность по памяти и по операциям копирования. В C# у вас, к примеру, выделось бы в куче под строку места с запасом и перевыделялось оно с копированием строки не каждый раз, а гораздо реже.

В целом нет ничего плохого в конкатенации коротких строк, кроме читабельности и плохой привычки так делать.
Гораздо читабельнее и эффективнее применить форматную строку, которые прекрасны в питоне. Или собрать куски строк в список, а потом единой операцией собрать их методом join, который тоже работает крайне эффективно: ', '.join(list_of_strings). Сджойнить кучу строк в списке через пустую строку гораздо эффективнее, чем наращивать её конкатенацией.

Если вы про соединение путей конкатенацией спрашиваете, то это тоже плохая идея, поскольку файловые системы бывают разные, с разными сепараторами пути, есть разные операционные системы более или менее толерантные к разным разделителям. А ещё можно напутать с абсолютными и относительными путями, с хвостовым слешем у каталога, с пустой левой частью конкатенации (''+'\'+'my_filename.ext'). 'Пустая строка, наверно, подразумевает текущий каталог, а уж никак не корневой. При такой конкатенации через слеш путь станет от корня.
Думаю во всех серьёзных языках есть стандартная библиотека с методами работы с путями. В питоне есть os.path (малость старенькая уже) и чудесная современная объектная библиотека pathlib. Вот она кроссплатформенная, содержит вкусный синтаксический сахар и приятные методы работа с файлами, каталогами и прочим. Всё в одном одобном флаконе.

[Andy_U]

Сергей Паньков, Про конкатенацию строк, посмотрите, пожалуйста, сюда. Совсем не всегда join быстрее, да и когда быстрее, то не сильно, особенно на последних версиях питона, это если отвлечься от проблем ранней оптимизации. А тогда использование именно канонического плюсика дает самый короткий и читаемый кода.

Про пути, в данном случае, что os, что pathlib - это из пушки по воробъям. А если еще учесть, что ошибка в Pycharm'е PY-30747 не исправляется уже полтора года...

[Сергей П]

Andy_U, окей, не всегда гораздо эффективнее. Нужно писать так, как читабельнее f(имхо f-строки вне конкуренции, а уж потом оптимизация, если прижмет. Но вот лично мне искренне непонятно кому это проще читать нагромождение кавычек и плюсиков..

[Andy_U]

Сергей Паньков,

См. этот ответ. :)

А кавычки вы сами предложили вынести наружу. А тогда канонический случай короче, если форматирование не требуется. Ну и f-строки повторно нельзя использовать, в отличие от .format():

if __name__ == '__main__':
    a = '123456'
    b = f'{a}'
    a = '4'
    print(b)

[Сергей П]

Andy_U,

См. этот ответ. :)

"your %s is in the %s" % (object, location)
or
"your " + object + " is in the " + location

Лично мне первый вариант более симпатичен и читабелен.
А вот этот ещё более:
f"your {object} is in the {location}"
Представьте, что во втором варианте вы ещё и кавычки хотите приконкатенировать вокруг одного или нескольких элементов. Это ж адище нечитабельности получается.
Да, большой минус, что повторное использование f-строк невозможно. Мне кажется стоило сделать менее "сахарную" интеграцию в виде какого-то метода у строки, который по умолчанию принимает целиком локальный контекст, но тогда пришлось бы делать такие же возможности для обычных функций, или оставить это грязным исключением. Возможность неявной передачи контекста, в свою очередь, грозит вылиться в проблемы с безопасностью, в утечки памяти, в неявные замыкания... Короче проблем огромной количество.
Для себя я выбрал следующие правила:

• если шаблон и подстановка одноразовые - f-строки.
  
• если шаблон нужно переиспользовать - .format()
  
• если нужно объединить явное небольшое количество строк: f-строка, .format или ''.join по обстоятельствам
  
• для большого или неизвестного числа строк ''.join
  

Плюсика в этом списке нет, поскольку использование этой перекрытой для строки операции кажется мне менее явным, чем все выше перечисленное.

Приоритеты:

1. читабельность и прозрачность
   
2. минимизация возможных ошибок
   
3. эффективность
   

[Andy_U]

Сергей Паньков,
По вашему первому сравнению вариантов. Такая строка обычно идет в print. А тогда канонический вариант сильно укорачивается в отличие от "современного":
print("your", object, "is in the", location)

Ну и нет совместимости с предыдущими версиями питона. А он еще ой как часто встречаются. Ну, и последние изменения в 3.8:

a = math.nan
print(f'{a=}')

наводят на мысль, что основное предназначение этих строк - быстрый отладочный вывод на консоль.

[Сергей П]

Andy_U,

наводят на мысль, что основное предназначение этих строк - быстрый отладочный вывод на консоль.

Вы сами себе противоречите.
Вот это и есть быстрый отладочный вывод:print("your", object, "is in the", location)
Причем там нельзя управлять разделительными пробелами, нельзя красиво отформатировать числа, нельзя красиво взять что-то в скобки... Короче устаревший способ что-то отформатировать для консоли.

канонический вариант сильно укорачивается в отличие от "современного"

И что? ну где-то он укорачивается, а где-то нам понадобится обрезать лишние цифры вещественного числа или выровнять по правому краю и вы воспользуетесь f-строкой и будет у вас зоопарк в коде.
И что вы сэкономите отказавшись от гибкости f-строк? Даже длиннее на несколько символов выйдет и менее очевидно в плане того, как это будет выглядеть на печати. Тут и запятые глаз мозолят и добавление пробелов принтом неявно и абзацем управлять надо отдельным параметром...

Ну и нет совместимости с предыдущими версиями питона. А он еще ой как часто встречаются.

Тут согласен, но я для себя твёрдо решил что, чем меньше он встречается, тем лучше и потакать ему я не собираюсь. Все мои проекты твёрдо и решительно переезжают на свежий питон, новые пишутся на новом со всеми плюшками. Мне лишний геморрой ник чему. Вот если делать пулл-реквесты в какую-то опенсорсную библиотеку с обратной совместимостью, то да, но в моём современном коде читабельность дороже.

наводят на мысль, что основное предназначение этих строк - быстрый отладочный вывод на консоль.

Мало ли на какие мысли вас наводят какие-то изменения. Мне кажется вы слишком много придаёте значения этой рефлексии.
Пользоваться свежим питончиком куда приятнее, расслабьтесь уже.

Такая строка обычно идет в print

У меня "такая строка" обычно идёт в logging.debug и в logging.info и в logging.warninig.
Это потому, что я много логирую. Но если что-то делается чуть реже чем часто, это не значит, что им можно пренебречь. Вы ещё соберите статистику для чего циклы и генераторы чаще используются и делайте из этого разные выводы.

[Andy_U]

Сергей Паньков,

Мало ли на какие мысли вас наводят какие-то изменения. Мне кажется вы слишком много придаёте значения этой рефлексии.

Это не я, а разработчики: bpo-36817

[Сергей П]

Andy_U, f'{x=}' мне тоже не нравится, кстати.

Answer 3

[Roman K]

Очень плохо

Answer 4

[Andy_U]

Я бы еще обернул shutil.make_archive в try-catch. Всякое может произойти. Типа файл заблокирован для чтения, стерся в процессе и т.д. и т.п.

Comments

[extroot]

Try-Except*

[Andy_U]

extroot, Да я тут еще с++ по необходимости вспоминаю.